Preserve correctness of `import.meta.url` in bundled modules. by usergenic · Pull Request #300 · Polymer/tools

usergenic · 2018-05-02T23:49:35Z

Note that in the case where an imported module's URL is identical to the bundle URL, no modification to the module code is needed before bundling it, but for all other cases where import.meta occurs somewhere in the document contents, we construct a module-scoped stand-in variable and swap other references to import.meta to use the stand-in, which is declared as an object-literal like {...import.meta, url: new URL(relativeUrlToModule, import.meta.url)}.

…de corrected URL.

… lerna bootstrap needed to do after I added acorn-import-meta.

… support.

usergenic · 2018-05-02T23:53:51Z

Fixes #294

rictic · 2018-05-02T23:55:12Z

packages/bundler/src/test/import-meta-url_test.ts

+  const aUrl = analyzer.resolveUrl('a.js')!;
+  const bUrl = analyzer.resolveUrl('subfolder/b.js')!;
+  const cUrl = analyzer.resolveUrl('c.js')!;
+  const dUrl = analyzer.resolveUrl('d.js')!;


Is dUrl used?

No, thanks! I was adding another test case and then gave up; forgot that was there. Removed.

rictic · 2018-05-02T23:56:27Z

packages/bundler/src/es6-rewriter.ts

+      moduleFile: babel.File,
+      relativeUrl: FileRelativeUrl): babel.File {
+    // Generate a stand-in for any local references to import.meta...
+    // const __bundledImportMeta = {...__importMeta, url: __bundledImportURL};


What is __importMeta here?

I previously had a local variable stand-in just for import.meta that is now gone. Old comment. Fixed to import.meta! Thanks!

rictic · 2018-05-02T23:57:54Z

packages/bundler/src/es6-rewriter.ts

+    traverse(newModuleFile, {
+      noScope: true,
+      MetaProperty: {
+        enter(path: NodePath) {


Can avoid the cast below by doing: enter(page: NodePath<babel.MetaProperty>) {

AH! Thanks! I have several of these in bundler code to fix! Will do now.

rictic · 2018-05-03T00:05:22Z

packages/bundler/src/es6-rewriter.ts

+              // URL has changed as a result of bundling.
+              const relativeUrl =
+                  ensureLeadingDot(this.bundler.analyzer.urlResolver.relative(
+                      this.bundle.url, id as ResolvedUrl));


Is the cast of id needed? It looks like it's a ResolvedUrl already

You're right. Fixed.

rictic · 2018-05-03T00:07:24Z

packages/bundler/src/es6-rewriter.ts

+                  ensureLeadingDot(this.bundler.analyzer.urlResolver.relative(
+                      this.bundle.url, id as ResolvedUrl));
+              const newAst = this._rewriteImportMetaToBundleMeta(
+                  document.parsedDocument.ast as babel.File, relativeUrl);


We could put this constraint into the type system if we added a type checking function that checks that document is a Document<ParsedJavascriptDocument>

You're right and in fact this code probably should have more guarantees/verification of the type of document it is loading anyways. I'll add a TODO here for this.

aomarks · 2018-05-03T00:18:13Z

packages/bundler/src/es6-rewriter.ts

+    // Generate a stand-in for any local references to import.meta...
+    // const __bundledImportMeta = {...__importMeta, url: __bundledImportURL};
+    const bundledImportMetaName = '__bundledImportMeta';
+    const bundledImportMetaDeclaration =


Could maybe use the @babel/template ast template string literal tag here instead of building it manually with the API? (Example: https://github.com/Polymer/tools/blob/master/packages/build/src/babel-plugin-dynamic-import-amd.ts#L64)

I love this idea. I even made a personal TODO note earlier today "Look up to see if there's a template string builder that would make this code cleaner."

I would like to consider bringing that approach to all of the ast production in bundler, but I'm going to hold off on a separate PR to bring that machinery in here. Adding TODO. It's a great idea and would make this code a lot better.

I should note that as of right now though, Bundler does not have any babel parsing in it and relies purely on Analyzer. Use of the template literal stuff would essentially require ensuring I either find a nice way to clone the Analyzer's babel parsing stack or rebuild it locally.

rictic · 2018-05-03T04:34:46Z

packages/bundler/src/es6-rewriter.ts

+                      babel.identifier('import'), babel.identifier('meta'))),
+                  babel.objectProperty(
+                      babel.identifier('url'),
+                      babel.newExpression(


We want to get the .href property of this new url, as import.meta.url is a string

DERP! Really good catch. Fixed.

…ter-the-fact casting.

…arsedJavascriptDocument> verification and add typings to its ast.

…-fix

usergenic · 2018-05-03T05:49:14Z

@rictic & @aomarks comments addressed

usergenic · 2018-05-03T06:00:15Z

Awww nuts. Something probably in the package-lock has updated causing something to break during gulp-typescript... https://ci.appveyor.com/project/Polymer/tools/build/1.0.858#L127

usergenic · 2018-05-03T06:00:47Z

Bunch of things like Property '_transform' in type 'GulpTransform' is not assignable to the same property in base type 'Transform'.

usergenic · 2018-05-03T06:11:38Z

Ah this failure is unrelated to branch. Created a new PR to start to fix the TS errors related to updating package locks at #309

justinfagnani · 2018-05-03T06:13:41Z

packages/bundler/src/es6-rewriter.ts

+    // Generate a stand-in for any local references to import.meta...
+    // const __bundledImportMeta = {...import.meta, url: __bundledImportURL};
+    // TODO(usergenic): Consider migrating this AST production mishmash into the
+    // `ast` tagged template literal available like this:


+1. Much easier to read and write.

justinfagnani · 2018-05-03T06:15:20Z

packages/bundler/src/es6-rewriter.ts

+    // TODO(usergenic): Consider migrating this AST production mishmash into the
+    // `ast` tagged template literal available like this:
+    // https://github.com/Polymer/tools/blob/master/packages/build/src/babel-plugin-dynamic-import-amd.ts#L64
+    const bundledImportMetaName = '__bundledImportMeta';


We should generate this name to be conflict free with identifiers at import.meta use sites.

Added issue. #310

justinfagnani · 2018-05-03T06:15:34Z

packages/bundler/src/es6-rewriter.ts

+                        babel.newExpression(
+                            babel.identifier('URL'),
+                            [
+                              //


remove? Or if it's for formatting, move to previous line?

clang-format went crazy so I put this in here to get it to behave. I can annotate further. This of course goes away after I switch to ast template literal...

justinfagnani · 2018-05-03T06:16:20Z

packages/bundler/src/es6-rewriter.ts

+                        babel.identifier('href')))
+              ]))
+        ]);
+    const newModuleFile = clone(moduleFile);


why do you have to clone it?

Because I modify in-place from document returned by analyzer and I'm trying to be a good citizen.

+1. Modifying documents returned by analyzer will lead to the worst kind of weird bugs with spooky action at a distance.

justinfagnani · 2018-05-03T06:17:20Z

packages/bundler/src/es6-rewriter.ts

+            return;
+          }
+          const bundledImportMeta = babel.identifier(bundledImportMetaName);
+          rewriteObject(metaProperty, bundledImportMeta);


use path.replaceWith instead?

Okay. That makes sense here. Replace'd

…-fix

rictic · 2018-05-03T18:31:51Z

packages/bundler/src/es6-rewriter.ts

+                        babel.identifier('href')))
+              ]))
+        ]);
+    const newModuleFile = clone(moduleFile);


+1. Modifying documents returned by analyzer will lead to the worst kind of weird bugs with spooky action at a distance.

keanulee · 2018-05-14T21:45:13Z

I'm seeing this transform present on the es6-bundled build output which doesn't make since it uses transformModulesToAmd and makes import undefined/an invalid keyword in FF. Is this expected, or is there some other JS config I need in that build configuration @usergenic ?

const bundledImportMeta = { ...import.meta,
  url: new URL('...', import.meta.url).href
};

usergenic · 2018-05-17T00:08:23Z

@keanulee the fix was to pin a specific dependency (that was suddenly causing errors after a release a couple days ago) until babel's minify preset is more stable #380

usergenic added 3 commits May 2, 2018 16:44

Add acorn-import-meta and rewrite import.meta when necessary to provi…

fdca32e

…de corrected URL.

Updated package-lock.json files for other packages because thats what…

1834a44

… lerna bootstrap needed to do after I added acorn-import-meta.

[bundler] Updated CHANGELOG to include import.meta.url preservation…

de81526

… support.

googlebot added the cla: yes label May 2, 2018

usergenic requested review from aomarks, justinfagnani and rictic May 2, 2018 23:49

usergenic mentioned this pull request May 2, 2018

Add import.meta transform in Bundler #294

Closed

rictic requested changes May 3, 2018

View reviewed changes

aomarks requested changes May 3, 2018

View reviewed changes

rictic reviewed May 3, 2018

View reviewed changes

usergenic added 5 commits May 2, 2018 22:17

[bundler] Remove unused variable from unit test file.

703265d

[bundler] Use explicit types for NodePath<babel.Something> instead af…

75722d0

…ter-the-fact casting.

Add TODO about getting some typechecking code in place for Document<P…

eb3232b

…arsedJavascriptDocument> verification and add typings to its ast.

import.met.url = new URL().href not new URL().

f7fe34f

Merge remote-tracking branch 'origin/master' into bundler-import-meta…

80e1ed1

…-fix

usergenic requested review from aomarks and rictic May 3, 2018 05:49

aomarks approved these changes May 3, 2018

View reviewed changes

usergenic mentioned this pull request May 3, 2018

Fix _transform in build to correspond to breakage from Typescript 2.8.3 Transform typings update #309

Merged

justinfagnani reviewed May 3, 2018

View reviewed changes

Merge remote-tracking branch 'origin/master' into bundler-import-meta…

cffd90b

…-fix

rictic approved these changes May 3, 2018

View reviewed changes

[bundler] Use path.replaceWith() instead of spooky rewriteObject()

528c431

usergenic merged commit f313b16 into master May 3, 2018

usergenic deleted the bundler-import-meta-fix branch May 3, 2018 19:02

Exnadella mentioned this pull request Nov 26, 2023

[Snyk] Fix for 5 vulnerabilities Exnadella/tools#70

Open

Conversation

usergenic commented May 2, 2018

Uh oh!

usergenic commented May 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

usergenic commented May 3, 2018

Uh oh!

usergenic commented May 3, 2018

Uh oh!

usergenic commented May 3, 2018

Uh oh!

usergenic commented May 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keanulee commented May 14, 2018

Uh oh!

usergenic commented May 17, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants